fix: do not let a cold-start deep link become the router's location - #673

No actionable comments were generated in the recent review. 🎉

Walkthrough

The change centralizes custom-scheme detection and applies it to cold-start router initialization and deep-link interception. Router tests now create and dispose a single router instance. New tests cover custom, web, relative, empty, and malformed locations.

Changes

Deep-link routing

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 28dca

Custom-scheme links now avoid the router's cold-start location while normal web and in-app navigation remain unchanged, preventing the reported startup crash. No actionable merge-blocking risk remains after normal checks and review.

Poem

I’m a rabbit with links in my tray

Custom schemes now find their way
Cold starts meet the router bright
Web URLs keep their rightful flight
Shared helpers make paths stay light

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Fixed in the pushed commit. startsWith('http') came from the predicate this consolidates — it was already written that way in both the interceptor and the redirect — so the looseness rode along into the shared helper.

Now compared exactly against http and https, with a regression test for httpfoo://example.com. I did not add a toLowerCase(): Uri already normalises the scheme, so there is a test asserting HTTPS://… is still treated as web rather than defensive code that cannot change the outcome.

Good catch, and you are right: the review found a real hole that my own device testing had hidden.

Once createRouter() discards the platform default, the cold-start link depends entirely on _processInitialDeepLink(). That path waited one frame plus a fixed 100 ms and then gave up if _router was still null — and _router is only created in the data branch of appInitializerProvider, so while Nostr/keys/sessions are still starting up the loading MaterialApp is on screen and there is no router to hand the link to. On my phone initialization won that race, which is all my manual test proved.

4d1a3fd1 replaces the timing guess with a stored link:

• lib/core/initial_deep_link_queue.dart — a small InitialDeepLinkQueue that holds the URI and delivers it exactly once, and only when a router is passed in.
• lib/core/app.dart — _processInitialDeepLink() stores the link and _drainInitialLink() hands it over in a post-frame callback; the same drain runs right after _router ??= createRouter(ref). The 100 ms delay is gone. A slow start now delays the order screen instead of dropping the link.

On the test: I pulled the coordination into its own class precisely so the delayed-router case could be tested, and test/core/initial_deep_link_queue_test.dart covers it — link stored, drained with no router (not delivered, still pending), drained again once the router exists (delivered), plus delivered-only-once. I did not write a widget test around MostroApp because pumping it means standing up the whole provider graph (appInitializerProvider, settings, auth, community, lifecycle) and mocking the app_links channel; the value would be in the wiring, and the cost in flakiness. Happy to add it if you would rather have it.

flutter analyze is clean on the touched files and flutter test test/core/ is green (55 tests). The full suite still shows the same 11 pre-existing load failures from the missing generated mocks (#606). I could not re-run the device check this time — the phone is not connected right now — so the on-device evidence is still the one from the earlier comment, which covers the crash but not the slow-init case.

Applied in f1782ac8: both error paths around the initial deep link now go through the logger singleton (logger.e with error/stackTrace) instead of debugPrint.

Note that one of the two, Error processing initial deep link, is pre-existing code — I only moved the other one out of the deleted _handleInitialMostroLink. I converted both since they sit in the same flow, but I left the rest of app.dart alone: it still uses debugPrint in seven other places, and rewriting those here would be unrelated churn in a bugfix PR. Worth a separate pass if you want the file consistent.

flutter analyze clean, flutter test test/core/ green (55 tests).

Thanks for the depth here — HIGH-1 and MEDIUM-1 were both real, and I verified each of your claims against the resolved SDK before touching anything. a0e4207d:

HIGH-1 — fixed, and the fix is not the one you suggested

You are right that addPostFrameCallback requests nothing (scheduler/binding.dart:792-797), and the failure path you describe is exact. I did not use ensureVisualUpdate() alone, because the delivery does not actually need a frame — it needs the navigator, which DeepLinkHandler._handleMostroDeepLink reads via router.routerDelegate.navigatorKey.currentContext (deep_link_handler.dart:88-104) and without which it logs and returns. So the condition is now that, not timing:

if (router.routerDelegate.navigatorKey.currentContext == null) {
  WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) _deliverPendingDeepLink(); });
  if (SchedulerBinding.instance.schedulerPhase == SchedulerPhase.idle) {
    // A post frame callback does not request a frame on its own.
    WidgetsBinding.instance.ensureVisualUpdate();
  }
  return;
}
_pendingDeepLink = null;
unawaited(_handleDeepLink(uri, router));

Your late-arrival case now delivers immediately, with no frame involved at all. ensureVisualUpdate() stays for the case where the router exists but the navigator is not mounted while the app is idle; it is guarded on schedulerPhase == idle so the retry path cannot request a frame from inside postFrameCallbacks and spin.

MEDIUM-1 — done, and checked by mutation

test/core/app_deep_link_test.dart mounts the real MostroApp with appInitializerProvider under a Completer, the app_links channel mocked, and a recording DeepLinkHandler. Four tests, and I ran them against the code they are supposed to protect:

• against f1782ac8: 3 of 4 fail (the late-arrival, the failed-delivery and the intercepted-stream cases).
• deleting the _deliverPendingDeepLink() call after _router ??= createRouter(ref) — the exact mutation you used: the first test fails too. Your point stands as written; it now does not.

One caveat worth stating: the completers must be created inside the test body, not in setUp, or they never complete for the widget under the fake async zone. Cost me an hour.

MEDIUM-2 — fixed

Delivery happens through a helper that restores the link on failure (_pendingDeepLink ??= uri) and logs. The test fails the first delivery and asserts the next rebuild still finds it.

MEDIUM-3 — fixed

customUrlStream no longer drops when _router == null; it goes through the same _queueDeepLink. Duplicates are already absorbed by DeepLinkHandler's 2 s window on _lastHandledDeepLinkUrl, so a link arriving through both paths is handled once.

LOW items

• LOW-1: all 7 remaining debugPrint calls are gone; grep -n debugPrint lib/core/app.dart is empty. On the link being dumped: DeepLinkInterceptor (logger.i, lines 23 and 27) and DeepLinkHandler:50 already log the full URI, so redacting only here would change nothing real — but logger only reaches the console when Config.isDebug (logger_service.dart:280-283), so the release exposure you flagged is closed. A redaction policy across the three sites is worth its own change.
• LOW-2: PR body now cites go_router-16.0.0/lib/src/router.dart:546-571. Confirmed against the lockfile.
• LOW-3: InitialDeepLinkQueue is deleted, along with its test. A Uri? on the state plus _deliverPendingDeepLink(), as you proposed.
• LOW-4: _processInitialDeepLink now queues any custom scheme via DeepLinkInterceptor.isCustomSchemeUri, matching exactly what createRouter discards, and the same predicate the intercepted path already used. A lightning: link would now reach DeepLinkHandler, which answers with the localized unsupportedLinkFormat instead of vanishing.

Verification

• flutter analyze lib/core test/core — only the 2 pre-existing containsSemantics infos.
• flutter test test/core/ — 56 passing.
• flutter test — 901 passing, the same 11 pre-existing load failures from the missing generated mocks (Dart run build_runner build fails on Flutter 3.44.0, source_gen 3.1.0 incompatible with analyzer 8.x #606).
• No device run: the phone is not connected right now, so the on-device evidence is still the earlier one, which covers the crash but not these timing windows.

Device verification, now that I could get the phone connected again (OnePlus 8T, Android 14, debug build of a0e4207d, live order a2561e55-d6d4-4d7c-9ce5-5a25378efac1).

Cold start — am force-stop first, so the process is really gone:

20:11:40.464  Initial deep link detected: mostro:a2561e55-…?relays=wss://relay.mostro.network
20:11:41.402  Handling deep link: mostro:a2561e55-…
20:11:43.107  Navigating to: /take_sell/a2561e55-… (Order: a2561e55-…, Type: sell)

The ~0.9 s between detection and handling is the link sitting in _pendingDeepLink while initialization finishes — the window that used to drop it. It opened that order, not the order book, and there is no assertion in the log.

App in foreground — the interceptor path, which is the window HIGH-1 was about, since the navigator is already mounted and nothing is asking for frames:

20:12:12.370  Intercepted custom URL: mostro:a2561e55-…
20:12:12.370  Handling deep link: mostro:a2561e55-…
20:12:12.493  Navigating to: /take_sell/a2561e55-…

Delivered in the same millisecond it arrived. Both paths fired here (initial link and interceptor) and DeepLinkHandler's 2 s window collapsed them into one navigation, as expected.

Plain launch, no link — order book, no deep-link log lines at all.

Not covered on device: a non-mostro: custom scheme (LOW-4). AndroidManifest.xml only registers mostro as an inbound filter, so there is no way to deliver one; that path is covered by the unit tests on isCustomSchemeUri only.

You were right about #1, and I took the exit you offered: this PR is now the crash fix only, and the delivery layer moved to #692.

On #1

The retain-on-failure guarantee was false, and the reason it looked true is the one you named: my double threw, DeepLinkHandler doesn't. I traced the five return paths you listed and they all return normally, so the catch was unreachable in production. A green test over a double whose contract differs from the collaborator is not evidence, and I shipped it as if it were.

The fix in #692 is not "retain on any failure" either — that would reopen an order the user already dismissed, which is your #10. handleInitialDeepLink returns Future<bool> and only "the app could not attempt it" (no navigator, or another link already being opened) is retained; anything the user already got an answer for counts as done. Retries ride on consecutive frames and stop after ten.

Two things fell out of writing that: the 2 s duplicate window was stamped before the context check, so a link that was never attempted came back and was taken for a duplicate of itself; and the same early return left the loading dialog up.

Split

• This PR: commits 1–2, plus Creating a new buy order (market rate/fixed price) #6 (the predicate moved to lib/core/deep_link_schemes.dart, so app_routes.dart no longer imports the interceptor and the _isCustomScheme alias is gone), Creating buy order with a lightning address #7 (the truncated doc), the log for the discarded platform default from Creating a new sell order (market rate/fixed price) #5, and Create a private chat for buyer and seller #11 (the router test holds one router and disposes it).
• fix: deliver a cold start deep link once the app can open it #692: User Log in and Posts Orders (Buy & Sell)  #1, Displays order list #2 (the == idle guard is gone — ensureVisualUpdate is already a no-op in the other phases, as you quoted), Decodes DMs from mostro #3, User log in with private key #4, Creating a new sell order (market rate/fixed price) #5's real fallback, Take sell order #8, Take buy order #9 and Connecting the buyer and the seller #10.

I also wrote into the body of this PR what the split costs: with only these commits the crash is gone, but on a slow cold start the link can still fail to open the order, since delivery is still main's 100 ms callback. On the device that window is ~0.9 s, so it is not theoretical. #692 closes it.

Not addressed

#2 has no test. I could not build one that distinguishes the phases — the navigator mounts in the same frame as the router — so it rests on the source contract you quoted rather than on a red-to-green.

Verification

flutter analyze lib/core test/core clean but for the two pre-existing containsSemantics infos; flutter test test/core/ green; full flutter test 897 passing with the same 11 pre-existing load failures (#606). On the OnePlus 8T: cold start opens the order with no assertion, foreground still works, plain launch shows the order book.